fix: non-breaking detection for redundant cast projections#5851
fix: non-breaking detection for redundant cast projections#5851albertosuman-1k5 wants to merge 3 commits into
Conversation
Signed-off-by: Alberto Suman <alberto.suman@1komma5grad.com>
|
Can we trigger again the workflows? Pretty sure the first run failed because of the issue described in #5852 |
rerunning... |
| # Everything other than the projection list must be structurally identical. Replacing each | ||
| # SELECT list with the same dummy literal lets the expression equality check focus on the | ||
| # FROM / WHERE / GROUP BY / ORDER BY / etc. parts of the query. | ||
| previous_skeleton = previous_query.copy() |
There was a problem hiding this comment.
This fallback can incorrectly treat projection inserts as non-breaking when the query uses positional references outside the SELECT list.
For example, this is currently categorized as NON_BREAKING on this branch:
SELECT a::DATE, s::TEXT FROM t ORDER BY 2
->
SELECT a::DATE, x::TEXT, s::TEXT FROM t ORDER BY 2
But ORDER BY 2 changes from sorting by s to sorting by x. The same issue applies to GROUP BY 2. Replacing both select lists with a dummy literal makes the skeleton comparison pass even though ordinal references still depend on the original projection positions.
|
|
||
| # Adding a UDTF projection (e.g. EXPLODE / UNNEST) can change row cardinality, so such a | ||
| # change is not safely non-breaking even when it appears as an extra SELECT item. | ||
| for projection in this_projections: |
There was a problem hiding this comment.
This UDTF guard misses aliased UDTF projections because EXPLODE(y) AS y is an exp.Alias, not an exp.UDTF.
When the fallback is triggered by same-type casts, this branch can categorize a change like this as NON_BREAKING:
SELECT a::DATE AS a, s::TEXT AS s FROM t
->
SELECT a::DATE AS a, x::TEXT AS x, EXPLODE(y) AS y, s::TEXT AS s FROM t
Upstream returns None for this case. The check probably needs to inspect newly added projections for nested UDTFs, while still allowing UDTFs inside scalar subqueries as the existing tests expect.
|
Could you add tests for queries that reference SELECT-list positions from other clauses, such as ordinal It would also be good to add coverage for aliased UDTF projections, since those may not appear as a top-level UDTF expression in the projection list but should still remain conservative. |
2 participants
Description
Summary
new_col::STRINGabove an existing::STRINGprojection.SqlModel.is_breaking_change()when SQLGlot’s AST diff emits spuriousMove/Updateedits.Context
SQLGlot’s tree diff can cross-match structurally similar cast nodes, especially identical
DataTypeleaves such asSTRING. This can make a purely additive projection change look like a non-Insertdiff, causing SQLMesh to returnNonefromis_breaking_change(). UnderAutoCategorizationMode.FULL, that becomesBREAKING.The new fallback verifies directly that:
SELECTs,Test plan
python -m pytest tests/core/test_snapshot.py -k "categorize_change_sql or categorize_change_seed" -qpython -m pytest cast_breaking_change_demo/test_cast_breaking_change.py -qpython -m pytest tests/core/test_snapshot.py tests/core/test_model.py::test_is_breaking_change tests/core/test_plan.py -qpre-commit run --files sqlmesh/core/model/definition.py tests/core/test_snapshot.py cast_breaking_change_demo/test_cast_breaking_change.pyChecklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO